build: set recursive option for globbing relocated files #49454
Conversation
This allow to use ** wildcard in zephyr_code_relocate cmake macro to match files in any subdirectory. Signed-off-by: Jan Pohanka <[email protected]>
Add details how the files are searched using zephyr_code_relocate cmake macro. Signed-off-by: Jan Pohanka <[email protected]>
zephyrbot
added
area: Build System
area: Kernel
area: Documentation
area: Userspace
zephyrbot
requested review from
carlescufi,
ceolin,
enjiamai,
gmarull,
jeremybettis,
mbolivar-nordic,
peter-mitsis,
tejlmand and
utzig
andyross
left a comment
andyross
left a comment
There was a problem hiding this comment.
Seems very reasonable, though technically this is an API change that will select different files for existing apps. This is a rarely-used feature though, so maybe not a big deal.
tejlmand
left a comment
tejlmand
left a comment
There was a problem hiding this comment.
Not opposed to the change or functionality, but have some concerns that must be addressed first, before a +1.
| mem_region, copy_flag, file_name = line.split(':', 2) | ||
|
|
||
| file_name_list = glob.glob(file_name) | ||
| file_name_list = glob.glob(file_name, recursive=True) |
There was a problem hiding this comment.
not opposed to the change itself, but do we have risk of side-effects ?
What happens in systems that might be building Zephyr inside Zephyr, or re-using source code.
Here a candidate could be TF-M which can build MCUboot as a secure bootloader.
Before approving this change, I would really like you to describe what problem you're solving, rather than just It might be handy.
Especially because the feature is not widely used, the risk is that side-effects are not discovered, as well as they could be hard to debug.
Should the proposed solution be configurable through an argument to this script ?
which then can be enable in Kconfig, similar to CONFIG_CODE_DATA_RELOCATION.
There was a problem hiding this comment.
I will look at the comments next week or the week after, once I'm back from the mountains...
There was a problem hiding this comment.
My main use case was this (the project runs on stm32h7x with plenty RAM free space and slow quadspi flash)
zephyr_code_relocate(${ZEPHYR_BASE}/subsys/**/*.c SRAM2_TEXT )
zephyr_code_relocate(${ZEPHYR_BASE}/drivers/**/*.c SRAM2_TEXT )
Relocating the zephyr system files lead to great speed up in my case. Specifying each subfolder in the example is definitely an option, but for me it increases complexity.
Original documentation of the zephyr_code_relocate macro is quite insufficient. When I read limited regexp support I had to go to the source and there I got the idea to add the recursive flag.
I agree it could possibly break some older code, but I have not found any usage now...
|
This PR start to look related to the effort carried out here: #50792 If so, please do not merge either of the PRs, until it has been settled which direction to take. |
|
#50792 is dependent on #50791, which adds @xhpohanka, I think this implementation might be more flexible, but I agree that a flag would be useful here, similar to the |
|
I personally think that these two approaches don't go against each other. My solution is based on idea that I want to relocate all files on some path. In my usecase it is easier, because I don't bother what libraries are there. The second one is thinking in opposite way (relocate a library and don't bother what files the library use). |
Not necessarily, but I wanted to ensure that especially your use-case was not covered by the library proposal.
So in this case, let's continue, but adjust your proposal. A disadvantage of the Python script is that it's much harder for users to directly debug which files got included. Instead the This will allow a user to do: or if only a single level is desired this will provide much more freedom to users as to how they want to glob files. Users can easily see which files are included for relocation by using This proposal will require a small change on how arguments in Combining this with support for generator expression, as commented here: This means a single solution should be able to support both ways. Side note: as part of this, we could consider to deprecate support for the regex in the python script itself, but that would be outside the scope for the above proposal. |
|
I totally agree, cmake solution would be better. |
|
@xhpohanka #50791 has been updated to support list of files. |
|
closing in favor of #50791 |
8 participants
It might be handy to select all files from a subdirectory when doing code relocation.